Conversation
|
kraenhansen
force-pushed
the
kh/mocha-remote
branch
19 times, most recently
from
d992ba6 to
55df470
Compare
kraenhansen
force-pushed
the
kh/mocha-remote
branch
from
55df470 to
caa8fe8
Compare
kraenhansen
force-pushed
the
kh/mocha-remote
branch
from
a994bd8 to
6f21e22
Compare
kraenhansen
force-pushed
the
kh/mocha-remote
branch
from
6f21e22 to
09f89d6
Compare
kraenhansen
added
Apple 🍎
kraenhansen
force-pushed
the
kh/mocha-remote
branch
from
09f89d6 to
5c74bb9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the CI check workflow to run the React Native test app on labeled Android/iOS runners and integrates mocha-remote for end-to-end tests in the app.
- Adds
test:androidandtest:iosscripts inapps/test-appusingmocha-remote+concurrently - Refactors
App.tsxto useMochaRemoteProvider/signals for test runner UI - Extends CI workflow (
.github/workflows/check.yml) with newtest-iosandtest-androidjobs conditional on labels
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/node-addon-examples/index.js | Temporarily commented out a crashing example |
| apps/test-app/package.json | Added Metro and test scripts, new test dependencies |
| apps/test-app/App.tsx | Switched to MochaRemoteProvider and signal-based UI |
| .github/workflows/check.yml | New CI jobs: iOS/Android test app runs based on PR labels |
Comments suppressed due to low confidence (4)
.github/workflows/check.yml:71
- [nitpick] The iOS test job installs Android SDK and NDK – consider removing this step from the iOS workflow to reduce setup time and eliminate irrelevant setup.
- name: Setup Android SDK
apps/test-app/package.json:10
- The
mocha-remoteflag is written as-- concurrentlybut should be--concurrently(no space) to correctly enable concurrent execution.
"test:android": "mocha-remote --exit-on-error -- concurrently --kill-others-on-fail --passthrough-arguments npm:metro 'npm:android -- {@}' --",
apps/test-app/package.json:11
- Likewise, change
-- concurrentlyto--concurrentlyso themocha-remotescript picks up the concurrently flag properly.
"test:ios": "mocha-remote --exit-on-error -- concurrently --passthrough-arguments --kill-others-on-fail npm:metro 'npm:ios -- {@}' --"
apps/test-app/App.tsx:11
- The import path
@react-native-node-api/node-addon-examplesdoes not match the dependency name (react-native-node-addon-examples) in package.json; update one to match the other.
import nodeAddonExamples from "@react-native-node-api/node-addon-examples";
| // TODO: This crashes (SIGABRT) | ||
| "async_work_thread_safe_function": () => require("./examples/5-async-work/async_work_thread_safe_function/napi/index.js"), | ||
| // "async_work_thread_safe_function": () => require("./examples/5-async-work/async_work_thread_safe_function/napi/index.js"), |
There was a problem hiding this comment.
[nitpick] Consider removing this long-commented-out example or extracting it behind a feature flag to avoid confusion and clean up the code.
| - name: Clone patched Hermes version | ||
| shell: bash | ||
| run: | | ||
| REACT_NATIVE_OVERRIDE_HERMES_DIR=$(npx react-native-node-api vendor-hermes --silent) |
There was a problem hiding this comment.
could being silent here hide any warnings that are early signs of issues?
| uses: android-actions/setup-android@v3 | ||
| with: | ||
| packages: tools platform-tools ndk;${{ env.NDK_VERSION }} | ||
| - run: rustup target add x86_64-linux-android aarch64-linux-android armv7-linux-androideabi i686-linux-android aarch64-apple-ios-sim |
There was a problem hiding this comment.
I'm guessing rustup just skips the nonsensical toolchain combinations (ios-sim on windows, etc)?
There was a problem hiding this comment.
I'm pretty sure you can cross-compile any of these. Or at the very least install the targets on any system.
Merging this PR will update the check workflow on CI to run the test app:
Also ...
I suggest using mocha-remote (a tool I maintain and use for Realm JS) to drive the "integrated unit tests", at least for now. I'm open to switching this out for another "universal testing library".